Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add body to ext auth #4671

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

AurelienPillevesse
Copy link

What type of PR is this?
Add gesture for body to ext auth

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #4431

@AurelienPillevesse AurelienPillevesse requested a review from a team as a code owner November 7, 2024 12:36
Signed-off-by: Aurélien Pillevesse <[email protected]>
Signed-off-by: Aurélien Pillevesse <[email protected]>
@AurelienPillevesse
Copy link
Author

The testing part (https://gateway.envoyproxy.io/contributions/develop/#testing) seems not to work on my computer so I tried to modify internal/gatewayapi/testdata/securitypolicy-with-extauth-backend.in.yaml and internal/gatewayapi/testdata/securitypolicy-with-extauth-backend.out.yaml manually and hope that it will pass.

Don't really know how to do it better.

@zirain
Copy link
Contributor

zirain commented Nov 7, 2024

it would be good to send API first.

@AurelienPillevesse
Copy link
Author

From api/ or internal/gatewayapi/ or both ?

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

Attention: Patch coverage is 14.28571% with 6 lines in your changes missing coverage. Please review.

Project coverage is 65.51%. Comparing base (aeb6848) to head (574815d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/xds/translator/extauth.go 0.00% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4671      +/-   ##
==========================================
- Coverage   65.54%   65.51%   -0.04%     
==========================================
  Files         211      211              
  Lines       31946    31953       +7     
==========================================
- Hits        20940    20935       -5     
- Misses       9762     9771       +9     
- Partials     1244     1247       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Aurélien Pillevesse <[email protected]>
Signed-off-by: Aurélien Pillevesse <[email protected]>
Signed-off-by: Aurélien Pillevesse <[email protected]>
Comment on lines +121 to +125
config.WithRequestBody = &extauthv3.BufferSettings{
MaxRequestBytes: 1024,
AllowPartialMessage: false,
PackAsBytes: false,
}
Copy link
Author

@AurelienPillevesse AurelienPillevesse Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arbitrary value (but must be greater than 0) for the MaxRequestBytes field, given that the AllowPartialMessage field is always equal to false.

https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ext_authz/v3/ext_authz.proto#extensions-filters-http-ext-authz-v3-buffersettings

@zhaohuabing
Copy link
Member

cc @guydc

Signed-off-by: Aurélien Pillevesse <[email protected]>
Copy link
Contributor

@guydc guydc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks good to me, just one comment on the docs and please rebase + fix test code generation

@@ -33,6 +33,10 @@ type ExtAuth struct {
// +optional
HeadersToExtAuth []string `json:"headersToExtAuth,omitempty"`

// BodyToExtAuth defines the Body to Ext Auth configuration.
// +optional
BodyToExtAuth *BodyToExtAuth `json:"bodyToExtAuth,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment about the maximum body size supported for this option? If I understand the docs correctly, when the body size is greater than 1024, the request would be blocked with status code 413 instead of being sent to ext-auth.

Copy link
Author

@AurelienPillevesse AurelienPillevesse Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my understanding, the maximum body size will not be take into account because in the BufferSettings object, AllowPartialMessage is hardcoded to false for the moment (so I put 1024 as random value).

extauthv3.BufferSettings{
	MaxRequestBytes:     1024,
	AllowPartialMessage: false,
	PackAsBytes:         false,
}

In my opinion, it's better the release this feature like that (so basic feature first) and in next iteration for this feature, allow the possibility for partial message.

So for the moment, all the body will be sent.

https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ext_authz/v3/ext_authz.proto#extensions-filters-http-ext-authz-v3-buffersettings

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm agree with that if the AllowPartialMessage values is false by default I think we can open another PR about that. This feature is very important because now we cannot add a body.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the following config:

static_resources:
  listeners:
    - address:
        socket_address:
          address: 0.0.0.0
          port_value: 8000
      filter_chains:
        - filters:
            - name: envoy.filters.network.http_connection_manager
              typed_config:
                "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
                codec_type: AUTO
                stat_prefix: ingress_http
                route_config:
                  name: local_route
                  virtual_hosts:
                    - name: upstream
                      domains:
                        - "*"
                      typed_per_filter_config:
                        envoy.filters.http.ext_authz:
                          "@type": type.googleapis.com/envoy.extensions.filters.http.ext_authz.v3.ExtAuthzPerRoute
                          check_settings:
                            with_request_body:
                              max_request_bytes: 1
                              allow_partial_message: false
                      routes:
                        - match:
                            prefix: "/"
                          route:
                            cluster: upstream-service
                http_filters:
                  - name: envoy.filters.http.ext_authz
                    typed_config:
                      "@type": type.googleapis.com/envoy.extensions.filters.http.ext_authz.v3.ExtAuthz
                      http_service:
                        server_uri:
                          uri: ext_authz
                          cluster: ext_authz-http-service
                          timeout: 0.250s
                  - name: envoy.filters.http.router
                    typed_config:
                      "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router

  clusters:
    - name: upstream-service
      type: STRICT_DNS
      lb_policy: ROUND_ROBIN
      load_assignment:
        cluster_name: upstream-service
        endpoints:
          - lb_endpoints:
              - endpoint:
                  address:
                    socket_address:
                      address: upstream-service
                      port_value: 8080

    - name: ext_authz-http-service
      type: STRICT_DNS
      lb_policy: ROUND_ROBIN
      load_assignment:
        cluster_name: ext_authz-http-service
        endpoints:
          - lb_endpoints:
              - endpoint:
                  address:
                    socket_address:
                      address: ext_authz-http-service
                      port_value: 9002

Specifying a body large enough will lead to request being rejected with 413 without being inspected by the ext-auth server.

curl -I http://localhost:8000
HTTP/1.1 403 Forbidden
date: Fri, 22 Nov 2024 15:30:44 GMT
server: envoy
content-length: 0

No body: rejected due to fail closed (ext auth doesn't really exist in this example).

curl -X POST http://localhost:8000 -d "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
[...]
< HTTP/1.1 413 Payload Too Large
< content-length: 17
< content-type: text/plain
< date: Fri, 22 Nov 2024 15:33:56 GMT
< server: envoy
< connection: close
Payload Too Large

Large body: rejected before being processed by ext auth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for with_request_body in SecurityPolicy.spec.extAuth
6 participants